Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create aliases for {a,e}fferent_{edges,nodes} #264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joni-herttuainen
Copy link
Contributor

Oftentimes, the naming of afferent_edges/efferent_edges/afferent_nodes/efferent_nodes cause confusion among our users. Also, quite frankly, I always have to look them up too as they don't feel intuitive. It's not clear what they mean and when is it needed to query by source nodes and when by target nodes.

To remove the ambiguity without introducing breaking changes, I created aliases for them (naming is open for discussion):

edges_by_source = efferent_edges
edges_by_target = afferent_edges
target_nodes_by_source = efferent_nodes
source_nodes_by_target = afferent_nodes

@joni-herttuainen joni-herttuainen added the enhancement New feature or request label Mar 11, 2024
@joni-herttuainen joni-herttuainen self-assigned this Mar 11, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d2d61c4) to head (2b741a1).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #264   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           33        33           
  Lines         2743      2751    +8     
=========================================
+ Hits          2743      2751    +8     
Flag Coverage Δ
pytest 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +631 to +634
edges_by_source = efferent_edges
edges_by_target = afferent_edges
target_nodes_by_source = efferent_nodes
source_nodes_by_target = afferent_nodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the naming a bit clunky. outgoing_edges and incoming_edges would be IMO more intuitive for eff and aff edges.

target_nodes_by_source also sounds very confusing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the naming a bit clunky. outgoing_edges and incoming_edges would be IMO more intuitive for eff and aff edges.

Sure, whatever makes sense to everybody is fine by me.

target_nodes_by_source also sounds very confusing!

What would you suggest? I can always elaborate it with get_target_nodes_of_source_nodes or whatever. I just want something that people will immediately understand so they don't accidentally choose the incorrect function for their purposes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just target_nodes and source_nodes?
They would match the names of the underlying libsonata edges population target_nodes and source_nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In libsonata, they expect edge ids as input (as opposed to node ids), right? Wouldn't that be even more confusing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants